Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Profiling tool : Profiling tool throws NPE when appInfo is null and unchecked #640

Merged
merged 2 commits into from
Jan 3, 2024

Conversation

kuhushukla
Copy link
Collaborator

Fixes #639 , needs a test , therefore draft

cindyyuanjiang
cindyyuanjiang previously approved these changes Oct 28, 2023
Copy link
Collaborator

@cindyyuanjiang cindyyuanjiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@@ -390,7 +390,9 @@ class Analysis(apps: Seq[ApplicationInfo]) {
val allRows = apps.flatMap { app =>
app.sqlIdToInfo.map { case (sqlId, sqlCase) =>
SQLDurationExecutorTimeProfileResult(app.index, app.appId, sqlId, sqlCase.duration,
sqlCase.hasDatasetOrRDD, app.appInfo.duration, sqlCase.problematic,
sqlCase.hasDatasetOrRDD,
if (app.appInfo != null) app.appInfo.duration else Option(0L),
Copy link
Collaborator

@parthosa parthosa Oct 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can we use option and map instead of if else in arguments to make it more idiomatic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the latest version? I see "Addressed" but the code is still using if-else-then?

@kuhushukla
Copy link
Collaborator Author

Please take a look @cindyyuanjiang , @parthosa

@kuhushukla kuhushukla requested a review from amahussein January 2, 2024 16:24
@kuhushukla
Copy link
Collaborator Author

Added headers for 2024 🎈

@amahussein
Copy link
Collaborator

Added headers for 2024 🎈
@kuhushukla
There is no commit with the updated copyright headers

Signed-off-by: Kuhu Shukla <[email protected]>
@kuhushukla
Copy link
Collaborator Author

Ok, we should be good now @cindyyuanjiang , @amahussein , apologies- my commit from yesterday ended with a bad push.

@amahussein amahussein added core_tools Scope the core module (scala) bug Something isn't working labels Jan 3, 2024
Copy link
Collaborator

@amahussein amahussein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kuhushukla
LGTME

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core_tools Scope the core module (scala)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Profiling tool throws NPE when appInfo is null and unchecked
4 participants